Skip to content

perf: Interval tree for managing segment metadata in memory#19138

Open
pirvtech wants to merge 36 commits into
apache:masterfrom
RivianVW-tech:segment-interval-tree
Open

perf: Interval tree for managing segment metadata in memory#19138
pirvtech wants to merge 36 commits into
apache:masterfrom
RivianVW-tech:segment-interval-tree

Conversation

@pirvtech

Copy link
Copy Markdown

Segment metadata stored in memory of the Historicals, is used when looking up segments that match an interval for query and segment loading purposes. Currently this is a serial scan that goes through all segments metadata in ascending start time order to find the right matching segments.

This changes introduces an Interval Tree as a more efficient way to store segment metadata in memory, to speed up searches for segments, that can potentially cut down search times from O(n) to O(logn).

Core changes to file processing/src/main/java/org/apache/druid/timeline/VersionedIntervalTimeline.java
Interval tree implementation in file processing/src/main/java/org/apache/druid/timeline/IntervalTree.java
Documentation comments have been included in important files and sections of code. Unit tests added.

This has been reviewed internally and is in a production Druid cluster.

@jtuglu1

jtuglu1 commented Mar 11, 2026

Copy link
Copy Markdown
Contributor

Hi – thanks. Do you have benchmarks for this?

@pirvtech

Copy link
Copy Markdown
Author

Hi – thanks. Do you have benchmarks for this?

I will add the benchmarks.

@kfaraz kfaraz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for putting this together, @pirvtech!
I have often felt the need for such a datastructure too, even outside the VIT (VersionedIntervalTimeline).

Thoughts on perf

  • I doubt if a single query is really going to benefit from this change since doing a contains() or an overlaps() check on say 25k intervals (which is a fairly large number of intervals for a typical Druid cluster) would not be very compute intensive.
    • You can think of 25k intervals as roughly 3 years worth of HOUR-granularity data.
  • But in high concurrency, this would still be beneficial since the VIT does all of its computations inside a giant lock. The shorter we hold the lock, the better.
  • Either way, we should add benchmarks as @jtuglu1 suggested for queries as well as VIT itself.

Notes on the implementation

I have left some inline comments.

Along with that, the approach would be much cleaner if you do something like this instead:

  • Add an IntervalNavigableMap<T> interface which extends NavigableMap<Interval, T>. This interface should have the following new methods:
    • entriesContaining(Interval interval)
    • entriesOverlapping(Interval interval)
    • entriesMatching(Interval interval) (Is this really needed?)
    • Alternatively, instead of methods that return a sub map, you could have methods that return a matching entry set.
  • Instead of HashMap and NavigableMap, the VIT class should use this new interface.
  • Add a class IntervalTreeMap<T> implements IntervalNavigableMap<T> extends TreeMap<Interval, T> and provides default implementations for the new methods.
    • For example, for the entriesContaining() method, we return the whole map.
  • Add a new class FastSearchIntervalMap<T> which performs the optimised search.
  • Based on value of the fastSearch flag passed to constructor of VIT, choose the implementation of the map.
  • This would ensure that there are minimal changes to VIT and we can easily swap out different map implementations.

Comment thread processing/src/main/java/org/apache/druid/timeline/VersionedIntervalTimeline.java Outdated
Comment thread processing/src/main/java/org/apache/druid/timeline/VersionedIntervalTimeline.java Outdated
@pirvtech

Copy link
Copy Markdown
Author

@kfaraz thanks for the comments. We were facing a scenario where the number of segments were in the order of millions, due to the amount of parallelization we have in ingestion tasks, the time spread of data being ingested, and relative time offset when a time interval gets compacted. This change helped our historicals be able to load and serve segments faster. I had made another change to make the scanning and loading of local on-disk segments during historical startup multi-threaded from a single threa that it was, but looks like someone beat us to the punch in submission :). I will review your code feedback and provide my responses.

@jtuglu1

jtuglu1 commented Apr 8, 2026

Copy link
Copy Markdown
Contributor

@kfaraz I think this PR will have much more impact than just Historical. See #19278. Broker timeline operations become terribly slow under high # of intervals. We can use the interval tree here to speed up those operations drastically and speed up queries under heavy segment load/remove callback load.

@jtuglu1

jtuglu1 commented Apr 8, 2026

Copy link
Copy Markdown
Contributor

@pirvtech – are you still working on this patch? We'd love to get this in and experimenting on Broker-side to fix some issues we've run into with large #s of intervals in the VIT. Happy to pick this up otherwise.

@kfaraz

kfaraz commented Apr 11, 2026

Copy link
Copy Markdown
Contributor

Thanks for the clarification, @jtuglu1 !
Could you share an estimate of the typical number of intervals that causes the slowness? Were you able to capture flamegraphs?

Regardless, I agree that this feature would be generally useful. We just need to clean up the changes a bit.

@jtuglu1

jtuglu1 commented Apr 13, 2026

Copy link
Copy Markdown
Contributor

Thanks for the clarification, @jtuglu1 ! Could you share an estimate of the typical number of intervals that causes the slowness? Were you able to capture flamegraphs?

Regardless, I agree that this feature would be generally useful. We just need to clean up the changes a bit.

I can get a flamegraph, but see the description of #19278 for explanation. Basically for a full year of MINUTE granularity you get ~500k intervals.

@FrankChen021 FrankChen021 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 10 of 10 changed files. The follow-up adds full-traversal APIs and documents the range-pruning contract; current PR call sites use range-monotonic predicates, so no inline reply is needed.


This is an automated review by Codex GPT-5

@pirvtech

Copy link
Copy Markdown
Author

Merged latest master into this branch and resolved conflicts.

@pirvtech

Copy link
Copy Markdown
Author

What are the next steps?

@pirvtech pirvtech changed the title Interval tree for managing segment metadata in memory perf: Interval tree for managing segment metadata in memory May 11, 2026

@FrankChen021 FrankChen021 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have reviewed the code for correctness, edge cases, concurrency, and integration risks; no issues found.

Reviewed 10 of 10 changed files.


This is an automated review by Codex GPT-5.5

@pirvtech

Copy link
Copy Markdown
Author

Any other comments @FrankChen021 @kfaraz @jtuglu1 or is it ready to merge.

@pirvtech

Copy link
Copy Markdown
Author

Any updates from reviewers if it is ready to merge?

@kfaraz kfaraz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updating the patch and for your patience, @pirvtech !

Overall, the changes look good. I have left some minor suggestions. A couple of files remain for review, will finish those soon as well.

Please also update the PR description to include a Release note section which describes the new config.

Comment thread processing/src/main/java/org/apache/druid/timeline/IntervalTree.java Outdated
Comment thread processing/src/main/java/org/apache/druid/timeline/IntervalTree.java Outdated
Comment thread processing/src/main/java/org/apache/druid/timeline/IntervalTree.java Outdated
Comment thread processing/src/main/java/org/apache/druid/timeline/IntervalTreeMap.java Outdated
import java.util.function.Consumer;
import java.util.stream.Collectors;

public class IntervalTreeTest

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use JUnit5 for the test.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like other tests in the project are using JUnit4. What needs to be done to move to JUnit5 (new dependency, annotations, imports?).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dependency should already be added. You can just start by using the JUnit5 @Test annotation and Assertions instead of Assert.

Comment thread processing/src/test/java/org/apache/druid/timeline/IntervalTreeMapTest.java Outdated
Comment thread server/src/main/java/org/apache/druid/segment/indexing/TimelineConfig.java Outdated
Comment thread server/src/main/java/org/apache/druid/server/SegmentManager.java Outdated
Comment thread processing/src/main/java/org/apache/druid/timeline/VersionedIntervalTimeline.java Outdated
@jtuglu1 jtuglu1 added this to the 38.0.0 milestone Jun 24, 2026
@pirvtech

pirvtech commented Jul 1, 2026

Copy link
Copy Markdown
Author

Thanks for updating the patch and for your patience, @pirvtech !

Overall, the changes look good. I have left some minor suggestions. A couple of files remain for review, will finish those soon as well.

Please also update the PR description to include a Release note section which describes the new config.

@kfaraz will look into your comments and address them.

@kfaraz

kfaraz commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

@pirvtech , the test failures seem genuine.
Also, there are 2 open comments.

Please try to get these resolved soon if this patch needs to be included in Druid 38.

@pirvtech

Copy link
Copy Markdown
Author

@kfaraz let me look at these

@pirvtech

Copy link
Copy Markdown
Author

@pirvtech , the test failures seem genuine. Also, there are 2 open comments.

Please try to get these resolved soon if this patch needs to be included in Druid 38.

Error seems to be about missing @Inject annotation on the TimelineConfig constructor. Not sure why guice is looking for the @Inject annotation on the single arg constructor or requiring a default constructor. The class is registered using JsonConfigProvider with the guice binder, and the JsonConfigProvider should be able to use the single arg connector as it has the @JsonCreator annotation on it. This seems to be ok for KafkaEmitterConfig.

@pirvtech

Copy link
Copy Markdown
Author

@pirvtech , the test failures seem genuine. Also, there are 2 open comments.
Please try to get these resolved soon if this patch needs to be included in Druid 38.

Error seems to be about missing @Inject annotation on the TimelineConfig constructor. Not sure why guice is looking for the @Inject annotation on the single arg constructor or requiring a default constructor. The class is registered using JsonConfigProvider with the guice binder, and the JsonConfigProvider should be able to use the single arg connector as it has the @JsonCreator annotation on it. This seems to be ok for KafkaEmitterConfig.

Figured why this is happening and added a default constructor

@Override
public @Nullable String toString()
{
return (root != null) ? root.print(1) : null;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

toString() should ideally not be null for a non-null object.
Return an empty string instead.

private final boolean fastIntervalSearch;

// Default constructor in cases where the class is not being constructed from JSON properties (such as direct
// injection from Guice without the JsonConfigProvider)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is it being injected by Guice directly?
We should not support this mode.

*/
public class SegmentTimelineConfig
{
/**

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this javadoc to the getter of this field instead.

}

@Override
public T remove(Object key)

Check notice

Code scanning / CodeQL

Confusing overloading of methods Note

Method IntervalTreeMap.remove(..) could be confused with overloaded method
remove
, since dispatch depends on static types.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants